Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Ingest --tags, --response-headers from CLI options #497

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ingest --tags, --response-headers from CLI options #497

wants to merge 1 commit into from

Conversation

sampsonj
Copy link

This PR addresses KEYCLOAK-12433, the --flags CLI option would not be parsed.
See JIRA issue for further details.

Changes include:

  • Allow the --flags and --response-headers CLI options to be ingested
  • Test that ensures all map based CLI options as defined in the config get parsed
  • decodeKeyPairs agnostic of value on right side of assignment (ie, can include the = character)
  • Test decodeKeyPairs updated to ensure value parsing is agnostic

golangcibot
golangcibot reviewed Dec 14, 2019
View reviewed changes
cli.go Show resolved Hide resolved
cli.go Show resolved Hide resolved
cli.go Show resolved Hide resolved
utils.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
cli_test.go Show resolved Hide resolved
@sampsonj sampsonj closed this Dec 14, 2019
@sampsonj sampsonj reopened this Dec 14, 2019
@stianst stianst requested a review from abstractj December 18, 2019 14:44
@slaskawi
Copy link

slaskawi commented Jan 9, 2020

@sampsonj LGTM

@abstractj Perhaps we should disable wsl linter? It's very picky..

@abstractj abstractj added this to the 9 milestone Jan 16, 2020
abstractj
abstractj reviewed Jan 21, 2020
View reviewed changes
Copy link

@abstractj abstractj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slaskawi @sampsonj sorry for the very late feedback, the PR fixes the issues with --tags which is great and thanks for that @sampsonj. But it also introduces the injection of --response-headers, something out of the scope for https://issues.redhat.com/browse/KEYCLOAK-12433.

Do we have any use case on why we should introduce the injection of --response-headers? I could confirm that the issue with tags is gone, but trying to inject --response-headers fails, at least for me. That means that we might be fixing the bug for tags and introducing a new bug with response-headers.

Maybe I'm wrong, but I'd rather to have anything that's out of scope for the injection of attributes on "Custom pages" discussed on the dev mailing list.

@abstractj
Copy link

@slaskawi regards wsl I'd say let's leave it for now and if it starts to becomes an annoyance we can disable.

@sampsonj
Copy link
Author

Thanks for having a look @slaskawi and @abstractj - much appreciated!

The reason for the increase in scope from the original JIRA ticket is due to the general nature of the unit test - it finds all map based properties as defined in the config and ensures they can be ingested from CLI flags. In writing that test, it identified response headers not being ingested and in order for that test to pass, I needed to add the --response-headers to the cli.go stanza that pulls in map based flags and merges with the config.
The utilization of that flag then happens upstream in middleware:
https://github.com/keycloak/keycloak-gatekeeper/blob/master/middleware.go#L379
Called from:
https://github.com/keycloak/keycloak-gatekeeper/blob/master/server.go#L197

As for the --response-headers flag not adding response headers, I don't get the same result as I am able to pass for example, --response-headers="X-Custom-Header=xcustomheadervalue" from the command line arguments, and have the header show up in the response. What might I be missing here?

Thanks for your time - let me know if I've missed something here and I'd be happy to push an update.

@stianst stianst removed this from the 9 milestone Apr 29, 2020
@abstractj abstractj mentioned this pull request Jun 4, 2020
Open
@abstractj abstractj changed the title [KEYCLOAK-12433] Ingest --tags, --response-headers from CLI options Ingest --tags, --response-headers from CLI options Jun 4, 2020
@abstractj abstractj removed their assignment Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@golangcibot golangcibot golangcibot left review comments

@abstractj abstractj abstractj left review comments

@slaskawi slaskawi slaskawi approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sampsonj @slaskawi @abstractj @golangcibot @stianst